-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a switch on dev and staging to force offline mode #12135
Conversation
I think this will do the job -
|
@thesahindia thanks for writing up those steps. It turns out we have a bug on production resulting from these steps 🙈 , so I'm going to come up with different steps. |
I ran into a problem on iOS where the api command fails when going back online, which suggests that a duplicate request is getting sent somehow. I'll look into it another day. iOS-fail.mp4 |
@rushatgabhane @aldo-expensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@thesahindia I'm finally ready for a review on this. @rushatgabhane I unassigned you since he is already assigned. |
This reverts commit e4d0e3f.
EOD for me. I will do the final testing in the morning. |
Ok, I'm hoping this is good to go now. |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2022-11-29.at.6.44.41.PM.movScreen.Recording.2022-11-29.at.5.53.50.PM.movScreen.Recording.2022-11-29.at.5.36.44.PM.movMobile Web - ChromeScreen.Recording.2022-11-29.at.7.17.40.PM.movScreen.Recording.2022-11-29.at.7.16.30.PM.movMobile Web - SafariScreen.Recording.2022-11-29.at.7.06.53.PM.movScreen.Recording.2022-11-29.at.7.05.52.PM.movDesktopScreen.Recording.2022-11-29.at.7.23.30.PM.movScreen.Recording.2022-11-29.at.7.21.25.PM.moviOSScreen.Recording.2022-11-29.at.7.04.33.PM.movScreen.Recording.2022-11-29.at.7.02.11.PM.movAndroidScreen.Recording.2022-11-29.at.7.13.30.PM.movScreen.Recording.2022-11-29.at.7.10.50.PM.mov |
Works well for me! Added a comment here. |
@aldo-expensify all you for a final review, thanks for all the help so far. |
Gentle bump @aldo-expensify |
Sorry, I'll review again today |
I'm not sure if this is a problem or not, but doing steps "C (web only)", I don't see "Failed to fetch"... where should I see that? I can see that the "Preview" and "Response" are empty, is that enough? Screen.Recording.2022-11-30.at.3.39.34.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested well, I agree with that it would be handy to have a shortcut or something, and I also agree with that this can be done in a follow up.
thanks for the work, it is useful to have an easy way of going offline in the IOS simulator
@thesahindia approved I think, should we merge? |
@aldo-expensify yeah the "failed to fetch" error only shows in the console, you can see that in my testing video. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @neil-marcellini in version: 1.2.36-0 🚀
|
🚀 Deployed to production by @francoisl in version: 1.2.36-4 🚀
|
Hey @neil-marcellini, this is ready for payment can we get someone to create an upwork job. Here's a post about the process. |
Thanks for letting me know and including a post about the process. @puneetlath is assigned to handle your payment. |
@thesahindia I sent you a hiring offer via Upwork. For me: https://www.upwork.com/nx/wm/pre-hire/c/8577561/offer/22655693 |
Paid! |
if (shouldForceOffline) { | ||
Log.info('[Pusher] Ignoring a Push event because shouldForceOffline = true'); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coming from #28063, also check shouldForceOffline
for sendEvent. : )
cc @marcaaron since you wrote a lot of the network code.
Details
It's not possible to turn the network on and off on the iOS simulator, so I have added a toggle in the test tool menu to force the network to be offline. The toggle will only show on dev and on staging.
When the toggle is on the keys
shouldForceOffline
andisOffline
are set totrue
under the network key in Onyx. When it is toggled offshouldForceOffline
is set tofalse
and we fetch the net info to setisOffline
. When forced offline API.write requests will not send and will be added to the persisted queue for when the app comes back online. DeprecatedAPI requests will not send and all other requests will fail to fetch.When the app is constructed the network is set to be offline, based on the value of
shouldForceOffline
. When the NetInfo state changes the offline status is set unlessshouldForceOffline
istrue
, which prevents the App from going online whenshouldForceOffline
is enabled. If you sign out while forcing offline, it will be disabled because otherwise there would be no way to go back online and sign in again, unless you refreshed the app.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/238637
PROPOSAL: N/A
Tests
A
B
C (web only)
D (web only)
ReconnectApp
is calledReconnectApp
is calledPusher
QA Steps
Same as QA.
PR Review Checklist
PR Author Checklist
I linked the correct issue in the
### Fixed Issues
section aboveI wrote clear testing steps that cover the changes made in this PR
Tests
sectionOffline steps
sectionQA steps
sectionI included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
toggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedIf a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar
, I verified the components usingAvatar
are working as expected)I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)If a new CSS style is added I verified that:
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avatar
is modified, I verified thatAvatar
is working as expected in all cases)If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
Web
A, B
web-A.B.mov
C, D
Web-C.D.mov
Pusher
Pusher.mov
Mobile Web - Chrome
android-chrome.mov
Mobile Web - Safari
ios-safari.mp4
Desktop
desktop.mp4
iOS
iOS.mp4
Pusher
iOS-Pusher.mov
Android
android.mov